Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: return all connected peers from REST API #2923

Merged
merged 4 commits into from
Jul 23, 2024
Merged

chore: return all connected peers from REST API #2923

merged 4 commits into from
Jul 23, 2024

Conversation

weboko
Copy link
Contributor

@weboko weboko commented Jul 19, 2024

Description

Currently GET /admin/v1/peers has additional logic to build a list of peers service node is connected to.
It does go through each group of protocols and builds a list:

nwaku/waku/waku_api/rest/admin/handlers.nim

Line 43 in 6d385ce

if not node.wakuRelay.isNil():

We can directly return list of connected peers as it more aligned with description of the endpoint:

Retrieve information about connected peers.

NagyZoltanPeter
NagyZoltanPeter requested changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but it is not working this way. We have a API response structure we must confirm with. So we return peers with their supported protocols.
This issue here - I think - that we won't return peers with protocol if the same protocol we don't support. (we = the node queried by GET /admin/peers). I think we should return all peers are connected and list all the protocols they support.

Also to mention this API endpoint has got no influence on how real connections are made, it is simply just a view of the current peer store.
Also there is the POST /admin/peers where we can inject new relay peers for the node.

@NagyZoltanPeter
Copy link
Contributor

@weboko : Please check, I updated your PR with the thought functional change.
My question if this is enough for a client (admin) purposes. Do we really need all peers with all protocols?
cc: @jm-clius

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 22, 2024 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2923

Built from d763cd5

@NagyZoltanPeter NagyZoltanPeter self-requested a review July 22, 2024 14:31
gabrielmer
gabrielmer approved these changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

Maybe we should add a test case for this?

@NagyZoltanPeter
Copy link
Contributor

LGTM, thank you!

Maybe we should add a test case for this?

Yepp, done!

@NagyZoltanPeter NagyZoltanPeter marked this pull request as ready for review July 23, 2024 10:25
@NagyZoltanPeter NagyZoltanPeter merged commit a29eca7 into master Jul 23, 2024
8 of 10 checks passed
@NagyZoltanPeter NagyZoltanPeter deleted the weboko/rest-peers branch July 23, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ivansete-status Ivansete-status Ivansete-status approved these changes

@gabrielmer gabrielmer gabrielmer approved these changes

@jm-clius jm-clius Awaiting requested review from jm-clius

@NagyZoltanPeter NagyZoltanPeter Awaiting requested review from NagyZoltanPeter

Assignees

@weboko weboko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@weboko @NagyZoltanPeter @gabrielmer @Ivansete-status